Skip to content

Conversation

@sjaeckel
Copy link
Member

@sjaeckel sjaeckel commented Oct 3, 2025

I've reordered and refactored some of the commits of #697 in order to limit its scope and simplify the merge and a potential faster release of 2.0

With these changes merged I currently don't see any planned ABI or API breakage (besides #515, but that's an entirely different discussion).

This should be the last PR with intentional changes before a v2.0.0-rc1, c.f. #568

@sjaeckel sjaeckel added this to the next milestone Oct 3, 2025
@sjaeckel sjaeckel requested review from karel-m and levitte October 3, 2025 09:22
@karel-m
Copy link
Member

karel-m commented Oct 3, 2025

I have just quickly changed the libtomcrypt in my CryptX perl bindings for the branch "some-improvements" from this PR.

It failed to build (which is probably expected) https://github.com/DCIT/perl-CryptX/actions/runs/18219248768/job/51875345151

I will try to adopt CryptX to the new changes (after that we will see whether CryptX test suite reveals something).

@sjaeckel sjaeckel force-pushed the some-improvements branch 2 times, most recently from 24f8a2f to 787e5c1 Compare October 3, 2025 13:39
@sjaeckel
Copy link
Member Author

sjaeckel commented Oct 3, 2025

I have just quickly changed the libtomcrypt in my CryptX perl bindings for the branch "some-improvements" from this PR.

Cool, thanks!

It failed to build (which is probably expected) https://github.com/DCIT/perl-CryptX/actions/runs/18219248768/job/51875345151

If I'm not mistaken there are two errors

[148] ff. is already caused by #524

[187] ff. is indeed caused by the API change of rsa_verify_hash_ex() in this PR

Both are expected, so thanks already in advance for fixing this in CryptX and testing those changes ☺️

[148] https://github.com/DCIT/perl-CryptX/actions/runs/18219248768/job/51875345151#step:7:149
[187] https://github.com/DCIT/perl-CryptX/actions/runs/18219248768/job/51875345151#step:7:188

@karel-m
Copy link
Member

karel-m commented Oct 5, 2025

I have updated my perl bindings and it seems to work fine.

Just one minor suggestion: please add @param mgf_hash_idx ..... to description of rsa_sign_hash_ex + rsa_verify_hash_ex

@sjaeckel sjaeckel force-pushed the some-improvements branch 2 times, most recently from 5b5fec8 to c760347 Compare October 16, 2025 08:31
@sjaeckel sjaeckel linked an issue Oct 16, 2025 that may be closed by this pull request
@sjaeckel sjaeckel force-pushed the some-improvements branch 4 times, most recently from e794721 to 151a655 Compare October 16, 2025 12:43
@karel-m
Copy link
Member

karel-m commented Oct 16, 2025

I am not sure about having: type = LTC_ECCSIG_RFC5656 (as I understand the "type" is rather a format specification not a flag defining whether to use deterministic signing) e.g. one might want LTC_ECCSIG_RFC5656 + LTC_ECCSIG_ETH27 at the same time.

@karel-m
Copy link
Member

karel-m commented Oct 16, 2025

ad struct ltc_ecc_sig_opts could we simplify int *recid; to int recid; ?

@sjaeckel
Copy link
Member Author

ad struct ltc_ecc_sig_opts could we simplify int *recid; to int recid; ?

Yeah, we could, but that would mean that it must be initialized to -1 if one is not interested in it (which is nearly always the case). With the current implementation via a pointer, it is disabled by default, so it'd be easier to use for the majority of users. But if you think it'd be better as int recid feel free to say so! I don't really have a hard opinion on that.

I am not sure about having: type = LTC_ECCSIG_RFC5656 (as I understand the "type" is rather a format specification not a flag defining whether to use deterministic signing) e.g. one might want LTC_ECCSIG_RFC5656 + LTC_ECCSIG_ETH27 at the same time.

I guess you're confusing RFC5656 and RFC6979 :)

@karel-m
Copy link
Member

karel-m commented Oct 17, 2025

I guess you're confusing RFC5656 and RFC6979 :)

Oh, yes. Sorry for the noise.

@karel-m
Copy link
Member

karel-m commented Oct 17, 2025

FYI current branch some-improvements seems to fail on arm and cywin (will investigate later)

@sjaeckel sjaeckel force-pushed the some-improvements branch 2 times, most recently from 3f05dbf to 747b9c1 Compare October 17, 2025 11:32
@karel-m
Copy link
Member

karel-m commented Oct 17, 2025

FYI current branch some-improvements seems to fail on arm and cywin (will investigate later)

it is a segfault (memory corruption) related to ecc_sign_hash_v2 (still not sure if in libtomcrypt or in C code of my perl bindings)

@sjaeckel
Copy link
Member Author

FYI current branch some-improvements seems to fail on arm and cywin (will investigate later)

it is a segfault (memory corruption) related to ecc_sign_hash_v2 (still not sure if in libtomcrypt or in C code of my perl bindings)

hmm

$ ./test ecc
[...]
SUCCESS: passed=1 failed=0 nop=0 duration=41.5sec real=41.5sec
$ file test                                              
test: ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), statically linked, BuildID[sha1]=8aab34cfb2c11a97baefcf2787a078587a6795ea, for GNU/Linux 4.3.0, with debug_info, not stripped

At least the tests can't reproduce it for AARCH64 with Qemu ...

@karel-m
Copy link
Member

karel-m commented Oct 17, 2025

on cywin it fails inside ecc_rfc6979_key in the first hmac_memory_multi (which is strange)

@karel-m
Copy link
Member

karel-m commented Oct 17, 2025

The fix DCIT/perl-CryptX@cccc52b

now passes tests also on cygwin + arm

@sjaeckel
Copy link
Member Author

sjaeckel commented Nov 2, 2025

After few hours I am still unable to switch my perl bindings to the new RSA API. I make it to compile but it fails during some operations (enc/dec, sign/verify) and it is not easy for me to debug why.

ugh, sorry! The tests seem to work fine for me.

I've just pushed my WIP state with the updated macros etc.

@sjaeckel
Copy link
Member Author

sjaeckel commented Nov 2, 2025

but still getting errors like "Invalid PK key or key type specified for function" + on other places errors like "Invalid hash specified" etc.

It could absolutely be that the generic validity checks don't handle everything correctly, I've only tested against the built-in set of tests... 😬

@karel-m
Copy link
Member

karel-m commented Nov 2, 2025

shouldn't the s_rsa_key_valid_pss_algs be rather s_rsa_key_valid_pss_oaep_algs and allow also padding LTC_PKCS_1_OAEP?

@karel-m
Copy link
Member

karel-m commented Nov 2, 2025

The simple example I am not able to implement in the new RSA API (sorry for just a code fragment extracted from my perl bindings):

/* load the key in DER format */
rsa_import(data, (unsigned long)data_len, &self->key);

/* try to encrypt */
ltc_rsa_op_parameters rsa_oparams = {
  .prng = &self->pstate,
  .wprng = self->pindex,
  .padding = LTC_PKCS_1_OAEP,
  .u.crypt.lparam = lparam_ptr,
  .u.crypt.lparamlen = (unsigned long)lparam_len,
  .params.hash_alg = hash_descriptor[lparam_hash_id].name,
  .params.mgf1_hash_alg = hash_descriptor[mgf_hash_id].name,
  .params.saltlen = 0,
  .params.pss_oaep = 1,
};
rv = rsa_encrypt_key_v2(data_ptr, (unsigned long)data_len, buffer, &buffer_len, &rsa_oparams, &self->key);

@sjaeckel
Copy link
Member Author

sjaeckel commented Nov 2, 2025

I'll have a look into it later, but please remove .params.pss_oaep = 1, and it works ... 🙈

@karel-m
Copy link
Member

karel-m commented Nov 2, 2025

I'll have a look into it later, but please remove .params.pss_oaep = 1, and it works ...

It worked. But IMO a bit surprising to me (as the whole concept of key->params / check->params / ltc_rsa_op_parameters ...)

Anyway, still bunch of other failures (will investigate later).

sjaeckel added a commit that referenced this pull request Nov 3, 2025
It expects a pair of type `(unsigned char*,unsigned long)` and not
`(unsigned char*,unsigned int)`.

Fixes: 46fa363 ("Finish up RFC6979 ECDSA keygen")
Reported-via: #699 (comment) ff.
Signed-off-by: Steffen Jaeckel <[email protected]>
@sjaeckel sjaeckel mentioned this pull request Nov 3, 2025
@sjaeckel
Copy link
Member Author

sjaeckel commented Nov 3, 2025

[...] a bit surprising to me (as the whole concept of key->params / check->params / ltc_rsa_op_parameters ...)

Yeah, a bit, but not really. The key->params exist because there's the possibility to restrict the PKCS#1 version via X.509 certificates. The re-use of ltc_rsa_parameters inside ltc_rsa_op_parameters, came out of laziness because it's kind of shared details (minus the pss_oaep field with isn't required there).

Regular RSA keys can be used with all PKCS#1 versions.

Restricted RSA keys ("PSS-keys") must only be used with PKCS#1 versions > 1.5, and come with a set of parameters. There's already rsa_import_x509.c::rsa_decode_parameters(), but that one isn't used yet, resp. I'll have to check whether it should be. I've other stuff to finish before continuing on this.

@sjaeckel sjaeckel changed the base branch from develop to minor-improvements November 3, 2025 13:20
Base automatically changed from minor-improvements to develop November 11, 2025 08:04
@sjaeckel sjaeckel mentioned this pull request Nov 18, 2025
2 tasks
@sjaeckel sjaeckel removed a link to an issue Nov 18, 2025
Signed-off-by: Steffen Jaeckel <[email protected]>
To be able to do a bit more, add an optional handler callback function.
Additional to that, also make it possible to mark elements as optional.

Signed-off-by: Steffen Jaeckel <[email protected]>
(and you should do that too)

Signed-off-by: Steffen Jaeckel <[email protected]>
@sjaeckel sjaeckel merged commit 03881e4 into develop Nov 18, 2025
78 checks passed
@sjaeckel sjaeckel deleted the some-improvements branch November 18, 2025 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants